Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wrap more inplace operations for fmpz and fmpq #1169

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Wrap more inplace operations for fmpz and fmpq #1169

merged 1 commit into from
Sep 8, 2021

Conversation

fingolfin
Copy link
Member

I just saw issue #1166 and that reminded me of this patch which I wrote some time ago but then never submitted...

Tests are failing, though, but I am not sure if that's me doing something wrong, or a bug in flint... specifically, I get this nonsense:

julia> c = fmpq(1); b=fmpq(23//11); add!(c,b,fmpq(1)) # this work
34//11

julia> c = fmpq(1); b=fmpq(23//11); add!(c,b,1) # this doesn't
1548059296613127//11

So am I using fmpq_add_si wrong?

thofma
thofma reviewed Sep 7, 2021
View reviewed changes
src/flint/fmpq.jl Outdated Show resolved Hide resolved
@thofma
Copy link
Member

thofma commented Sep 7, 2021

@tthsqe12 Are you also adding those in #1168?

@fingolfin
Copy link
Member Author

@thofma thanks for fixing my stupid bug :-).

Just to be clear: if #1168 already supersedes this, I won't complain if this PR just gets closed -- but perhaps something in here is useful.

I also still think that we need more supporting APIs to allow writing truly generic code using these inplace APIs, at the very least mutable_copy resp. copy_if_mutable functions (see Nemocas/AbstractAlgebra.jl#1000)

@tthsqe12
Copy link
Contributor

tthsqe12 commented Sep 7, 2021 •
edited
Loading

Yes, I am. I currently have

set!
swap!
zero!
one!
add!
sub!
addeq!
subeq!
mul!
addmul!
submul!
...

@fingolfin
Copy link
Member Author

There is another Ref{Int} and I'll add a few more tests to really cover all these previously broken functions

@tthsqe12
Copy link
Contributor

tthsqe12 commented Sep 7, 2021

It's ok. I will take care of these thing in my pr. The main thing is we need to decide on whatever the deepcopy/mutable/make mutable interface is.

@fingolfin
Copy link
Member Author

Thing is, deepcopy has a different scope than "make a mutable copy" and "copy if mutable" methods have. We are abusing it for making mutable copies right now, but neither is it sufficient for that, nor is it easy to use efficiently in all situations. So I really think we need to do some design work -- and the easiest may be to heavily borrow from MutableArithmetics.jl, or outright adopt it (which does not mean we need to break existing code, we can still provide add! etc. etc. -- but yeah, we'd have to audit and change all code using deepcopy or providing deepcopy methods, I am afraid. But this is independent of MA.

@codecov
Copy link

codecov bot commented Sep 7, 2021 •
edited
Loading

Codecov Report

Merging #1169 (ee7d964) into master (5b37f25) will increase coverage by 0.00%.
The diff coverage is 67.74%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1169   +/-   ##
=======================================
  Coverage   86.48%   86.48%           
=======================================
  Files          71       71           
  Lines       26659    26690   +31     
=======================================
+ Hits        23055    23084   +29     
- Misses       3604     3606    +2
Impacted Files Coverage Δ
src/flint/fmpz.jl 88.50% <50.00%> (-0.54%) ⬇️
src/flint/fmpq.jl 93.16% <78.94%> (+1.08%) ⬆️
src/flint/fmpz_laurent_series.jl 84.61% <0.00%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b37f25...ee7d964. Read the comment docs.

@tthsqe12
Copy link
Contributor

tthsqe12 commented Sep 7, 2021

This looks fine. We can merge this, and then I can rebase mine. I don't have have all of these in mine any more.

@thofma thofma merged commit 4096f8d into Nemocas:master Sep 8, 2021
@fingolfin fingolfin deleted the mh/mutable branch September 16, 2021 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thofma thofma thofma left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fingolfin @thofma @tthsqe12